Scalar path silently swallows errors in ANSI mode

High Severity

In the scalar evaluation path of DecimalRescaleCheckOverflow, when fail_on_error is true (ANSI mode), rescale_and_check correctly returns an Err, but .ok() on line 207 silently converts it to None. This causes the scalar path to return a null value instead of propagating the overflow error. The array path correctly propagates errors via ? on try_unary. This means ANSI-mode overflow errors are silently swallowed for scalar inputs, producing null instead of an error.

value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! The call to .ok() will hide any Err by silently converting it to None, and that None will lead to retuning SQL NULL result. Prevents a bug where an error is silently suppressed

CheckOverflow skip ignores its own data_type and fail_on_error

Medium Severity

When CheckOverflow wraps a WideDecimalBinaryExpr, the code returns the child directly without verifying that CheckOverflow's data_type (precision/scale) matches the WideDecimalBinaryExpr's output type. If Spark's plan specifies a different precision/scale in CheckOverflow than the binary expression's return_type, the output type would be wrong. The fail_on_error flag from CheckOverflow is also silently discarded.

value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! Before fusing (by using WideDecimalBinaryExpr) the logic should check that the Cast's precision/scale match the requested output precision/scale pair. Prevents calculating wrong rescaling if they don't match

The evaluate method calculates scale_factor using 10i128.pow(abs_delta as u32). Since abs_delta is derived from the difference of two i8 scales provided in the Spark plan, it can exceed 38 (the maximum power of 10 that fits in a signed 128-bit integer). For example, if output_scale is 38 and input_scale is -1, abs_delta would be 39, causing a panic and crashing the executor. This represents a Denial of Service (DoS) risk if an attacker can provide a Spark plan with extreme scale values.

Recommendation: Use checked_pow and handle the overflow by returning an error or null, or validate the scales during the planning phase.

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! Apache Arrow supports decimals with negative scales. The subtraction of the scales here could lead to a bigger result (x - (-y) == x + y) that could lead to overflowing the maximum allowed scale of 38. Checks should be added to prevent such overflows and/or panics.

The precision_bound helper function uses 10i128.pow(precision as u32). If the output_precision (a u8 value from the Spark plan) is greater than 38, this call will panic. While Spark typically limits decimal precision to 38, Comet should validate this input to avoid crashing the executor on malformed or malicious plans.

Recommendation: Validate that precision is within the supported range (1-38) before calling pow, or use checked_pow.

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! Apache Arrow supports decimals with negative scales. The subtraction of the scales here could lead to a bigger result (x - (-y) == x + y) that could lead to overflowing the maximum allowed scale of 38. Checks should be added to prevent such overflows and/or panics.

The i256_pow10 function uses wrapping_mul to compute scaling factors. If the exponent exp exceeds 77 (the maximum power of 10 that fits in a 256-bit integer), the result will overflow and wrap around, leading to incorrect scaling factors. This can occur if the difference between decimal scales is extreme. Subsequent arithmetic operations using these incorrect factors will produce wrong results, posing a data integrity risk.

Recommendation: Use checked arithmetic or validate that the exponent is within the safe range (0-77) before performing the calculation.

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! A check should be added to prevent trying to use exponents bigger than 77. Otherwise the multiplication operation will wrap and lead to wrong calculations.

The variables _p1, _s1, _p2, and _s2 are prefixed with an underscore, which typically indicates they are unused. However, they are used in the if guard of this match arm. This can be confusing for future readers. It would be clearer to remove the underscore prefix from these variable names to signal that they are intentionally used.

For improved readability and idiomatic Rust, consider using value.signum() to get the sign of the number. While the current implementation is correct, signum() is the standard library function for this purpose.

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The i128 type has a signum() method that returns a similar result. The only difference is that it returns 0 for 0, not 1 as the current code.

⚠️ Potential issue | 🟡 Minor

Tighten fusion precondition by validating cast output type too.

The fusion currently validates only cast.child input type. Please also require the Cast output type to match the CheckOverflow target decimal type before replacing the pair.

                 if let Some(cast) = child.as_any().downcast_ref::<Cast>() {
                     if let (
                         DataType::Decimal128(p_out, s_out),
                         Ok(DataType::Decimal128(_p_in, s_in)),
-                    ) = (&data_type, cast.child.data_type(&input_schema))
+                        Ok(DataType::Decimal128(cast_p, cast_s)),
+                    ) = (
+                        &data_type,
+                        cast.child.data_type(&input_schema),
+                        cast.data_type(&input_schema),
+                    )
                     {
-                        return Ok(Arc::new(DecimalRescaleCheckOverflow::new(
-                            Arc::clone(&cast.child),
-                            s_in,
-                            *p_out,
-                            *s_out,
-                            fail_on_error,
-                        )));
+                        if cast_p == *p_out && cast_s == *s_out {
+                            return Ok(Arc::new(DecimalRescaleCheckOverflow::new(
+                                Arc::clone(&cast.child),
+                                s_in,
+                                *p_out,
+                                *s_out,
+                                fail_on_error,
+                            )));
+                        }
                     }
                 }

Verify each finding against the current code and only fix it if needed.

In `@native/core/src/execution/planner.rs` around lines 395 - 409, The fusion
replaces a Cast+CheckOverflow pair without verifying the Cast's output type;
update the precondition in the block that matches
child.as_any().downcast_ref::<Cast>() so it also checks that
cast.data_type(&input_schema) equals the target DataType::Decimal128(p_out,
s_out) (i.e., the same precision/scale as the CheckOverflow target) before
creating DecimalRescaleCheckOverflow; locate the matching logic around data_type
and cast.child.data_type(&input_schema) and add the extra equality check on
cast.data_type(&input_schema) to tighten the fusion precondition.

value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! Before fusing (by using WideDecimalBinaryExpr) the logic should check that the Cast's precision/scale match the requested output precision/scale pair. Prevents calculating wrong rescaling if they don't match

⚠️ Potential issue | 🟠 Major

Propagate ANSI overflow errors in scalar path (don’t silently null them).

The scalar branch currently uses .ok() on rescale_and_check, which drops errors and returns None. That makes fail_on_error = true behave like legacy nulling for scalars.

             ColumnarValue::Scalar(ScalarValue::Decimal128(v, _precision, _scale)) => {
-                let new_v = v.and_then(|val| {
-                    rescale_and_check(val, delta, scale_factor, bound, fail_on_error)
-                        .ok()
-                        .and_then(|r| if r == i128::MAX { None } else { Some(r) })
-                });
+                let new_v = match v {
+                    Some(val) => {
+                        let r = rescale_and_check(val, delta, scale_factor, bound, fail_on_error)?;
+                        if !fail_on_error && r == i128::MAX {
+                            None
+                        } else {
+                            Some(r)
+                        }
+                    }
+                    None => None,
+                };
                 Ok(ColumnarValue::Scalar(ScalarValue::Decimal128(
                     new_v, p_out, s_out,
                 )))
             }

Verify each finding against the current code and only fix it if needed.

In `@native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs` around
lines 204 - 213, The scalar Decimal128 branch is currently calling
rescale_and_check(...).ok() which swallows errors and turns ANSI overflow (when
fail_on_error is true) into a null; change this to propagate errors instead of
dropping them: replace the .ok().and_then(...) chain in the
ColumnarValue::Scalar(ScalarValue::Decimal128(...)) arm with a match on
rescale_and_check(...) that returns Err(e) when the call returns Err(e), and for
Ok(r) map r == i128::MAX to None (legacy null) or Some(r) otherwise, then
construct the Decimal128 scalar with that result; reference rescale_and_check,
ColumnarValue::Scalar, and ScalarValue::Decimal128 to locate the code to update.

value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The call to .ok() will hide any Err by silently converting it to None, and that None will lead to retuning SQL NULL result. Prevents a bug where an error is silently suppressed

⚠️ Potential issue | 🟡 Minor

Validate child count in with_new_children to avoid panics.

Direct indexing with children[0] can panic if a malformed rewrite passes the wrong arity.

     fn with_new_children(
         self: Arc<Self>,
         children: Vec<Arc<dyn PhysicalExpr>>,
     ) -> datafusion::common::Result<Arc<dyn PhysicalExpr>> {
+        if children.len() != 1 {
+            return Err(DataFusionError::Internal(format!(
+                "DecimalRescaleCheckOverflow expects 1 child, got {}",
+                children.len()
+            )));
+        }
+
         Ok(Arc::new(DecimalRescaleCheckOverflow::new(
             Arc::clone(&children[0]),
             self.input_scale,
             self.output_precision,
             self.output_scale,
             self.fail_on_error,
         )))
     }

Verify each finding against the current code and only fix it if needed.

In `@native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs` around
lines 224 - 234, The with_new_children method currently indexes children[0] and
can panic for the wrong arity; update with_new_children (on
DecimalRescaleCheckOverflow) to first validate children.len() == 1 and return an
Err(datafusion::common::Result::Err(...)) when the count is wrong instead of
indexing directly; then construct the new Arc by using the validated children[0]
(or children.get(0).unwrap()) and call DecimalRescaleCheckOverflow::new with the
same self.input_scale, self.output_precision, self.output_scale, and
self.fail_on_error values.

⚠️ Potential issue | 🟡 Minor

Add arity checks in with_new_children for panic safety.

This method assumes exactly two children and will panic on invalid rewrite inputs.

-use datafusion::common::Result;
+use datafusion::common::{DataFusionError, Result};
@@
     fn with_new_children(
         self: Arc<Self>,
         children: Vec<Arc<dyn PhysicalExpr>>,
     ) -> Result<Arc<dyn PhysicalExpr>> {
+        if children.len() != 2 {
+            return Err(DataFusionError::Internal(format!(
+                "WideDecimalBinaryExpr expects 2 children, got {}",
+                children.len()
+            )));
+        }
+
         Ok(Arc::new(WideDecimalBinaryExpr::new(
             Arc::clone(&children[0]),
             Arc::clone(&children[1]),
             self.op,
             self.output_precision,
             self.output_scale,
             self.eval_mode,
         )))
     }

Verify each finding against the current code and only fix it if needed.

In `@native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs` around lines
272 - 283, The with_new_children implementation for WideDecimalBinaryExpr
assumes exactly two children and can panic; update with_new_children to validate
children.len() == 2 and return a proper Err when the arity is incorrect instead
of indexing into children. Locate the method named with_new_children on
WideDecimalBinaryExpr and before calling WideDecimalBinaryExpr::new, check the
number of children and return an appropriate Result error (preserving existing
error type used in this crate) when it is not 2; otherwise construct the new
instance using Arc::clone(&children[0]) and Arc::clone(&children[1]) with the
same self.op, self.output_precision, self.output_scale, and self.eval_mode.

scale_factor = 10i128.pow(abs_delta as u32) can overflow/wrap for larger scale deltas (e.g., casts involving negative scales), which would make the rescale+overflow check incorrect or panic in debug builds.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:useful; category:bug; feedback: The Augment AI reviewer is correct! Apache Arrow supports decimals with negative scales. The subtraction of the scales here could lead to a bigger result (x - (-y) == x + y) that could lead to overflowing the maximum allowed scale of 38. Checks should be added to prevent such overflows and/or panics.

In the scalar path, rescale_and_check(...).ok() drops errors; if fail_on_error is true (ANSI), an overflow would become a NULL result instead of propagating an error.

Severity: high

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:useful; category:bug; feedback: The Augment AI reviewer is correct! The call to .ok() will hide any Err by silently converting it to None, and that None will lead to retuning SQL NULL result. Prevents a bug where an error is silently suppressed